feat: add `fs/copy-file` #2201

HRIDYANSHU054 · 2024-04-23T17:09:09Z

adds file system copy file utility to Stdlib

Description

What is the purpose of this pull request?

This pull request:

is aligned with the purpose of achievinng feature parity with Node.js fs package. It Brings file system's powerful copy file utility to Stdlib expanding its existing fs utilities. It also tends to make the copy File utility version independent for node. Moreover, with this I also wish to know with what approach do we further add abstractions over the fs utilities.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Implementation details

-- async version

function onCopy( error ) {
    if ( error ) {
        throw error;
    }
    console.log( 'src.txt has been copied to dest.txt' );
}

copyFile( 'src.txt', 'dest.txt', onCopy );

-- sync version

var copyFile = require( '@stdlib/fs/copy-file' );

// Explicitly handle the error...
var err = copyFileSync( 'src.txt', 'dest.txt' );
if ( err instanceof Error ) {
    throw err;
}

more --> currently researching on how fs/constants could be implemented for stdlibsuch that further feature parity and version independency are achievable.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

Read, understood, and followed the contributing guidelines.

@stdlib-js/reviewers

HRIDYANSHU054 · 2024-04-25T12:44:10Z

Hi @Planeshifter could you rerun the CI checks so that I can rectify anything that isn't correct?

HRIDYANSHU054 · 2024-04-29T12:52:23Z

@kgryte , @Planeshifter could you rerun the CI check

Planeshifter · 2024-05-19T22:13:08Z

/stdlib update-copyright-years

kgryte · 2024-10-17T08:50:46Z

lib/node_modules/@stdlib/fs/copy-file/bin/cli

+
+	mode = 0;
+	if ( args.length < 2 && ( !flags.source || !flags.destination ) ) {
+		return cli.error( new Error( 'Both source and destination files must be provided' ) );


This error message should be updated based on common CLI error conventions.

kgryte · 2024-10-17T08:53:50Z

lib/node_modules/@stdlib/fs/copy-file/bin/cli

+	if ( args.length < 2 && ( !flags.source || !flags.destination ) ) {
+		return cli.error( new Error( 'Both source and destination files must be provided' ) );
+	}
+	if ( flags.source ) {


Argument handling is somewhat convoluted. Either --source and --destination flags are provided OR positional arguments. Further, at L80, we resolve relative to the cwd, but not if flags are provided. We should be consistent.

kgryte · 2024-10-17T08:54:52Z

lib/node_modules/@stdlib/fs/copy-file/bin/cli

+	}
+
+	// Perform copying...
+	copyFile( src, dest, mode, onCopy );


Using async copy in the CLI is not necessary. Using sync is fine and can be faster in common CLI usage contexts.

kgryte · 2024-10-17T08:55:30Z

lib/node_modules/@stdlib/fs/copy-file/docs/repl.txt

@@ -0,0 +1,55 @@
+
+{{alias}}( src, dest[, mode], clbk )
+    Asynchronously copies src to dest.


This description should be updated to avoid abbreviations.

kgryte · 2024-10-17T08:55:44Z

lib/node_modules/@stdlib/fs/copy-file/docs/repl.txt

+        Modifiers for copy operation. Default: 0.
+
+    clbk: Function
+        Callback to invoke upon copying src file to dest file.


Same comment.

kgryte · 2024-10-17T08:55:54Z

lib/node_modules/@stdlib/fs/copy-file/docs/repl.txt

+    ...     if ( err ) {
+    ...         console.error( err.message );
+    ...     }
+    ...     console.log('src.txt has been copied to dest.txt');


Missing spaces.

kgryte · 2024-10-17T08:56:14Z

lib/node_modules/@stdlib/fs/copy-file/docs/repl.txt

+
+
+{{alias}}.sync( src, dest[, mode] )
+    Synchronously copies src to dest.


Same comment as above.

kgryte · 2024-10-17T08:56:49Z

lib/node_modules/@stdlib/fs/copy-file/examples/index.js

+}
+
+// Asynchronously copy data from src.txt to dest.txt:
+


Suggested change

kgryte · 2024-10-17T08:59:06Z

lib/node_modules/@stdlib/fs/copy-file/lib/sync.js

+	if ( !isString( dest ) && !isBuffer( dest ) ) {
+		throw new TypeError( format( 'invalid argument. Second argument must be either a string or a Buffer. Value: `%s`.', dest ) );
+	}
+	if ( !isNumber( mode ) && mode !== null ) {


Above we state that the mode must be an integer; however, here, we use a weaker check. Furthermore, are only nonnegative integers permitted?

kgryte · 2024-10-17T08:59:31Z

lib/node_modules/@stdlib/fs/copy-file/lib/sync.js

+*
+* @param {(string|Buffer)} src - source filename to copy
+* @param {(string|Buffer)} dest - destination filename of the copy operation
+* @param {integer} [mode] - modifiers for copy operation


Missing default parameter value.

kgryte · 2024-10-17T08:59:47Z

lib/node_modules/@stdlib/fs/copy-file/lib/sync.js

+// MAIN //
+
+/**
+* Synchronously copies `src` to `dest`. By default, `dest` is overwritten if it already exists.


Second sentence should be moved to a note.

kgryte · 2024-10-17T09:00:35Z

lib/node_modules/@stdlib/fs/copy-file/lib/async.js

+var fs = require( 'fs' );
+var createReadStream = require( 'fs' ).createReadStream;
+var createWriteStream = require( 'fs' ).createWriteStream;


Three separate imports is not necessary. Use a single import and then refer to the appropriate methods.

kgryte · 2024-10-17T09:00:56Z

lib/node_modules/@stdlib/fs/copy-file/lib/async.js

+*
+* @param {(string|Buffer)} src - source filename to copy
+* @param {(string|Buffer)} dest - destination filename of the copy operation
+* @param {(integer)} [mode] - modifiers for copy operation.


Suggested change

* @param {(integer)} [mode] - modifiers for copy operation.

* @param {integer} [mode=0] - modifiers for copy operation

kgryte · 2024-10-17T09:01:17Z

lib/node_modules/@stdlib/fs/copy-file/lib/async.js

+		writeStream = createWriteStream( dest );
+		readStream.on( 'error', onReadError );
+		writeStream.on( 'error', onWriteError );
+		writeStream.on( 'finish', onFinish);


Suggested change

writeStream.on( 'finish', onFinish);

writeStream.on( 'finish', onFinish );

kgryte · 2024-10-17T09:01:55Z

lib/node_modules/@stdlib/fs/copy-file/lib/async.js

+	* @private
+	*/
+	function onFinish() {
+		cb( null ); // Success


Do we even need to provide an argument here?

Hridyanshu7 added 6 commits April 23, 2024 20:15

Added CopyFile utility to Stdlib/fs

c61af98

fixed a cli test

20f004d

some CI checks fixed

107afd0

some CI errors fixed

d3e0ab3

some CI checks in Run changed examples solved

29c68b8

some CI checks in Run changed examples solved_attempt(2)

bcf23dd

Planeshifter changed the title ~~Added fs/copy file~~ feat: add fs/copy Apr 24, 2024

Hridyanshu7 added 3 commits April 24, 2024 12:46

Some validation improvements and ci errors solved.

9fec68f

Some CI checks fixed

f57b555

CI errors solved

dce41a7

kgryte changed the title ~~feat: add fs/copy~~ feat: add fs/copy-file May 8, 2024

Planeshifter self-requested a review May 8, 2024 19:43

stdlib-bot and others added 4 commits May 19, 2024 22:13

chore: update copyright years

447fbc6

chore: minor clean-up and use feature detection

5968203

test: add tests for when native functions are unavailable

a044112

chore: use fs.constants consistently

1d42f32

kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Oct 17, 2024

kgryte reviewed Oct 17, 2024

View reviewed changes

lib/node_modules/@stdlib/fs/copy-file/examples/index.js

}

// Asynchronously copy data from src.txt to dest.txt:

Copy link

Member

kgryte Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

kgryte reviewed Oct 17, 2024

View reviewed changes

kgryte added the Needs Changes Pull request which needs changes before being merged. label Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `fs/copy-file` #2201

feat: add `fs/copy-file` #2201

HRIDYANSHU054 commented Apr 23, 2024 •

edited

Loading

HRIDYANSHU054 commented Apr 25, 2024

HRIDYANSHU054 commented Apr 29, 2024

Planeshifter commented May 19, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

kgryte Oct 17, 2024

		@@ -0,0 +1,55 @@

		{{alias}}( src, dest[, mode], clbk )
		Asynchronously copies src to dest.



		{{alias}}.sync( src, dest[, mode] )
		Synchronously copies src to dest.

	* @param {(integer)} [mode] - modifiers for copy operation.
	* @param {integer} [mode=0] - modifiers for copy operation

	writeStream.on( 'finish', onFinish);
	writeStream.on( 'finish', onFinish );

feat: add fs/copy-file #2201

Are you sure you want to change the base?

feat: add fs/copy-file #2201

Conversation

HRIDYANSHU054 commented Apr 23, 2024 • edited Loading

Description

Questions

Other

Checklist

HRIDYANSHU054 commented Apr 25, 2024

HRIDYANSHU054 commented Apr 29, 2024

Planeshifter commented May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `fs/copy-file` #2201

feat: add `fs/copy-file` #2201

HRIDYANSHU054 commented Apr 23, 2024 •

edited

Loading